Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adding a preserve mtime feature to copy_to_directory and copy_directory #898

Merged
merged 1 commit into from
Aug 11, 2024

Conversation

jpinkul
Copy link
Contributor

@jpinkul jpinkul commented Aug 10, 2024

This change adds the preserve_mtime attribute to the copy_to_directory and copy_directory rules. When set the copied files will have their modified time set to the same value as the original source.

The motivation for adding this feature is that I was building a third party library from source using rules_foreign_cc and tried to use copy_to_directory to merge in a configuration header with the upstream source. Without this feature all of the modify times were reset and this caused the build to erroneously determine that generated files packaged with the source were out of date. With this preserve_mtime feature I was successfully able to build the third party library using the released version of these files.

Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

The copy_directory and copy_to_directory rules now support preserving the original modification time of their sources.

Test plan

  • Temporarily updated the two rules to use the version of the tools form the repository by uncommenting the development only tool sections.
  • A a new test_preserve_mtime under e2e/smoke test was added that verifies that the modified times are the same in the destination as the source.
  • The preserve_mtime = True attribute was temporally removed from the targets that are used by the test. It was verified the old behavior still occurred and the test failed with an appropriate error message.
  • All existing tests under e2e were executed.

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2024

CLA assistant check
All committers have signed the CLA.

@jpinkul jpinkul changed the title Adding a preserve mtime feature to copy_to_directory and copy_directory feat: Adding a preserve mtime feature to copy_to_directory and copy_directory Aug 10, 2024
@kormide kormide merged commit 74ac451 into bazel-contrib:main Aug 11, 2024
25 of 26 checks passed
@alexeagle
Copy link
Collaborator

Hey @jpinkul this is flaky, and getting in the way of getting a green run on main - do you have a few minutes to look?

@alexeagle
Copy link
Collaborator

@jpinkul
Copy link
Contributor Author

jpinkul commented Aug 15, 2024

Oh and it broke the BCR release too https://buildkite.com/bazel/bcr-presubmit/builds/7213#0191522d-6b85-45c8-962a-4f5e426e6e3c

@alexeagle sorry about that! I changed the test to go_test in #907 for hopefully better portability. Can these CI builds be run on this PR to verify it works as expected in the other environments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants